management/server/types/settings_test.go (1)

5-20: ⚡ Quick win

Consider adding a ConnectionMode deep-copy test to cover cloneStringPtr

The test validates cloneUint32Ptr via P2pRetryMaxSeconds, but cloneStringPtr (used by ConnectionMode) is a separate code path with no test coverage. A mirror test would be low-effort:

🧪 Suggested addition

+func TestSettings_Copy_ConnectionMode(t *testing.T) {
+	mode := "p2p-dynamic"
+	src := &Settings{ConnectionMode: &mode}
+	dst := src.Copy()
+	if dst.ConnectionMode == nil {
+		t.Fatal("Copy lost ConnectionMode pointer")
+	}
+	if *dst.ConnectionMode != "p2p-dynamic" {
+		t.Fatalf("expected p2p-dynamic, got %s", *dst.ConnectionMode)
+	}
+	// Verify deep copy (different pointers).
+	*dst.ConnectionMode = "relay-forced"
+	if *src.ConnectionMode != "p2p-dynamic" {
+		t.Fatal("Copy did not deep-clone ConnectionMode")
+	}
+}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@management/server/types/settings_test.go` around lines 5 - 20, Add a new unit
test mirroring TestSettings_Copy_P2pRetryMaxSeconds to validate deep-copy
behavior for Settings.ConnectionMode: create a source Settings with
ConnectionMode set to a string pointer, call Settings.Copy(), assert
dst.ConnectionMode is non-nil and equals the original value, then mutate
*dst.ConnectionMode and assert *src.ConnectionMode remains unchanged to verify
cloneStringPtr produced a deep copy; reference the Settings struct, its Copy
method, and cloneStringPtr in the test.

shared/management/http/api/openapi.yml (1)

357-369: ⚡ Quick win

Mark legacy lazy_connection_enabled as deprecated and state precedence.

With both fields present, migration intent is unclear unless deprecation and precedence are explicit in the schema.

🧭 Suggested compatibility clarification

         lazy_connection_enabled:
           x-experimental: true
+          deprecated: true
           description: Enables or disables experimental lazy connection
           type: boolean
           example: true
         connection_mode:
           x-experimental: true
           type: string
           enum: [relay-forced, p2p, p2p-lazy, p2p-dynamic]
           nullable: true
           description: |
             Account-wide default peer-connection mode. NULL means
             "fall back to lazy_connection_enabled" for backwards compatibility.
+            When non-NULL, `connection_mode` takes precedence.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shared/management/http/api/openapi.yml` around lines 357 - 369, Mark the
legacy boolean field lazy_connection_enabled as deprecated and make precedence
explicit: add deprecated: true to the lazy_connection_enabled schema and update
its description to indicate it is legacy and overridden by connection_mode when
connection_mode is non-null; update the connection_mode description to
explicitly state that a non-null connection_mode takes precedence over
lazy_connection_enabled and that NULL means "fall back to
lazy_connection_enabled" for backwards compatibility (use the existing keys
lazy_connection_enabled and connection_mode to locate the schema).

shared/connectionmode/mode_test.go (1)

77-94: 💤 Low value

ModeFollowServer missing from TestToLazyConnectionEnabled.

The table omits ModeFollowServer, so its ToLazyConnectionEnabled() return value is never asserted. If its behaviour changes it won't be caught. Consider adding a case (expected value is almost certainly false, matching ModeUnspecified).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@shared/connectionmode/mode_test.go` around lines 77 - 94, The test
TestToLazyConnectionEnabled omits ModeFollowServer so its
ToLazyConnectionEnabled() behavior isn't asserted; add a test case entry in the
cases table for ModeFollowServer with the expected boolean (likely false,
matching ModeUnspecified) so the call to Mode.ToLazyConnectionEnabled() for
ModeFollowServer is validated like the other entries.

client/internal/peer/status.go (1)

367-382: 💤 Low value

UpdatePeerIceBackoff silently ignores unknown peer keys — inconsistent with other update methods.

Every other UpdatePeer* method (e.g., UpdatePeerState, UpdateWireGuardPeerState, UpdatePeerFQDN) returns an error when the peer is not found. UpdatePeerIceBackoff returns nothing and silently no-ops. While a silent no-op is safe here (the peer may have been removed between the ICE failure and this call), the divergence from the established pattern makes it harder to detect misconfigured call sites during development. Consider returning an error for consistency, or add a brief comment explaining why the silent no-op is intentional.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/status.go` around lines 367 - 382, UpdatePeerIceBackoff
currently silently no-ops when the peer key is missing, diverging from other
update methods like UpdatePeerState / UpdateWireGuardPeerState / UpdatePeerFQDN
which return an error; change UpdatePeerIceBackoff(pubKey string, snap
BackoffSnapshot) to return an error, lock/unlock as before, and when peer not
found return a descriptive error (e.g., fmt.Errorf("peer %s not found",
pubKey)); update callers to handle the returned error, or if you intentionally
want the silent no-op instead, add a short comment above UpdatePeerIceBackoff
explaining why ignoring missing peers is acceptable to avoid surprise.

client/internal/peer/conn_test.go (1)

374-384: 💤 Low value

Test assertion is tied to an internal error string.

Line 381 checks strings.Contains(errMsg, "workerICE") — if the error message in AttachICE is ever rewarded, this assertion passes vacuously. Consider asserting on a sentinel error value (errors.Is) instead of a substring, or at minimum add a comment acknowledging the coupling.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/conn_test.go` around lines 374 - 384, The test currently
asserts on the error string from c.AttachICE() by checking for "workerICE",
which is brittle; change the implementation of AttachICE to return or wrap a
sentinel error (e.g., ErrNilWorkerICE) and update the test to use errors.Is(err,
ErrNilWorkerICE) instead of substring matching; if adding a sentinel isn't
possible right now, at minimum replace the substring check with errors.Is
against an existing exported sentinel or add a clear comment documenting the
coupling. Ensure AttachICE (and any callers) wrap the sentinel with
fmt.Errorf("%w: ...", ErrNilWorkerICE) so errors.Is will work reliably.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

client/internal/peer/worker_ice.go (1)

218-231: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

lastKnownState is still accessed without consistent synchronization — data race.

IsConnected() reads lastKnownState while holding muxAgent, but the writes inside onConnectionStateChange at lines 550 and 562 happen on the pion callback goroutine without acquiring muxAgent. This is a Go data race, and the new Conn.onNetworkChange path (in conn.go at line 1244) now reads through IsConnected() to decide whether to close the agent — a torn read can either skip Close() on a freshly-disconnected agent or close a still-Connected one, exactly the scenario the comment in onNetworkChange says to avoid.

Switch lastKnownState to atomic.Int32 (or similar) so reads and writes are consistent regardless of muxAgent:

-	// we record the last known state of the ICE agent to avoid duplicate on disconnected events
-	lastKnownState ice.ConnectionState
+	// Accessed from pion's state-change callback and from IsConnected()
+	// concurrently — keep atomic so reads on the network-change path are
+	// not torn against unlocked writes in onConnectionStateChange.
+	lastKnownState atomic.Int32 // ice.ConnectionState

-		lastKnownState:    ice.ConnectionStateDisconnected,
+	}
+	w.lastKnownState.Store(int32(ice.ConnectionStateDisconnected))

 func (w *WorkerICE) IsConnected() bool {
 	w.muxAgent.Lock()
 	defer w.muxAgent.Unlock()
-	return w.agent != nil && w.lastKnownState == ice.ConnectionStateConnected
+	return w.agent != nil && ice.ConnectionState(w.lastKnownState.Load()) == ice.ConnectionStateConnected
 }

 		case ice.ConnectionStateConnected:
-			w.lastKnownState = ice.ConnectionStateConnected
+			w.lastKnownState.Store(int32(ice.ConnectionStateConnected))
 			w.logSuccessfulPaths(agent)
@@
-			if w.lastKnownState == ice.ConnectionStateConnected {
-				w.lastKnownState = ice.ConnectionStateDisconnected
+			if ice.ConnectionState(w.lastKnownState.Load()) == ice.ConnectionStateConnected {
+				w.lastKnownState.Store(int32(ice.ConnectionStateDisconnected))
 				w.conn.onICEStateDisconnected(sessionChanged)
 			}

Also applies to: 549-563

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/worker_ice.go` around lines 218 - 231, IsConnected reads
lastKnownState under muxAgent but onConnectionStateChange writes it without
locking, causing a data race; change the WorkerICE.lastKnownState from a plain
int to an atomic.Int32 (or similar), update IsConnected() to use
lastKnownState.Load() (while keeping muxAgent for agent nil-check), and update
onConnectionStateChange to use lastKnownState.Store(newState) instead of
assigning directly; ensure all other accesses (e.g., in the code paths around
lines referenced) use atomic Load/Store and remove any unsynchronized direct
reads/writes to lastKnownState while leaving agent access protected by muxAgent
as before.

client/internal/peer/conn.go (1)

202-212: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Disable-backoff state is still not preserved across Open/Close cycles.

The ConnConfig comment at lines 98-100 says ^uint32(0) is the disable sentinel and the resolver translates it to 0 before reaching here, while the Open() block treats 0 as "use built-in default" (DefaultP2PRetryMax). These two semantics conflict: a peer that the resolver wanted disabled ends up with the default 15 m backoff. Likewise, SetIceBackoffMax(0) writes 0 into config.P2pRetryMaxSeconds, and a subsequent Close()/Open() cycle silently re-promotes the peer from "disabled" to the default schedule.

Use a distinct in-process representation for "disabled" so the three states (use default, disabled, explicit value) can each round-trip. For example, treat the wire ^uint32(0) as the in-process "disabled" sentinel rather than collapsing it to 0 at the resolver, so Open() can branch on it directly.

+const p2pRetryMaxDisabled = ^uint32(0)
+
-	backoffCap := time.Duration(conn.config.P2pRetryMaxSeconds) * time.Second
-	if backoffCap == 0 {
-		backoffCap = DefaultP2PRetryMax
-	}
+	var backoffCap time.Duration
+	switch conn.config.P2pRetryMaxSeconds {
+	case 0:
+		backoffCap = DefaultP2PRetryMax
+	case p2pRetryMaxDisabled:
+		backoffCap = 0
+	default:
+		backoffCap = time.Duration(conn.config.P2pRetryMaxSeconds) * time.Second
+	}
@@
 func (conn *Conn) SetIceBackoffMax(d time.Duration) {
 	conn.mu.Lock()
 	defer conn.mu.Unlock()
-	conn.config.P2pRetryMaxSeconds = uint32(d / time.Second)
+	if d == 0 {
+		conn.config.P2pRetryMaxSeconds = p2pRetryMaxDisabled
+	} else {
+		conn.config.P2pRetryMaxSeconds = uint32(d / time.Second)
+	}
 	if conn.iceBackoff != nil {
 		conn.iceBackoff.SetMaxBackoff(d)
 	}
 }

The companion change in the resolver/engine.go must also stop translating ^uint32(0) → 0 — pass the sentinel through verbatim.

Also applies to: 1152-1163

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/conn.go` around lines 202 - 212, The code collapses the
wire sentinel for "disabled" into 0 which conflicts with Open() treating 0 as
"use default", causing disabled peers to be re-enabled; fix by introducing a
distinct in-process sentinel for "disabled" (e.g. a named constant like
DisabledP2pRetrySentinel) and preserve the resolver wire sentinel instead of
translating it to 0 in resolver/engine.go, then update Open()'s logic around
conn.config.P2pRetryMaxSeconds to: if it equals the disabled sentinel, configure
conn.iceBackoff as disabled (or leave nil) so backoff is turned off; else if it
equals 0 treat it as "use default" and set backoffCap = DefaultP2PRetryMax,
otherwise use the explicit value to newIceBackoff / SetMaxBackoff; also ensure
SetIceBackoffMax writes the same in-process sentinel when disabling so
Close()/Open() round-trips.

client/android/preferences.go (1)

349-352: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Negative secs is silently aliased to a huge uint32; for SetP2pRetryMaxSeconds it collides with the disable sentinel.

A SetRelayTimeoutSeconds(-1) becomes 4294967295 seconds (~136 years), and the same bad cast on SetP2pRetryMaxSeconds(-1) yields exactly ^uint32(0), which is the documented wire-format sentinel for "user-explicit disable backoff" (see client/internal/peer/conn.go lines 98-100). Negative input therefore silently flips backoff into the disabled state without any validation. Clamp or reject < 0 in all three setters.

 func (p *Preferences) SetRelayTimeoutSeconds(secs int64) {
+	if secs < 0 {
+		secs = 0
+	}
 	v := uint32(secs)
 	p.configInput.RelayTimeoutSeconds = &v
 }
@@
 func (p *Preferences) SetP2pTimeoutSeconds(secs int64) {
+	if secs < 0 {
+		secs = 0
+	}
 	v := uint32(secs)
 	p.configInput.P2pTimeoutSeconds = &v
 }
@@
 func (p *Preferences) SetP2pRetryMaxSeconds(secs int64) {
+	if secs < 0 {
+		secs = 0
+	}
 	v := uint32(secs)
 	p.configInput.P2pRetryMaxSeconds = &v
 }

Also applies to: 370-373, 390-393

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/android/preferences.go` around lines 349 - 352, SetRelayTimeoutSeconds
(and the two other setters that cast int64 to uint32, e.g.,
SetP2pRetryMaxSeconds and the comparable setter) currently cast negative secs
into a huge uint32 (which can collide with the disable sentinel); update each
setter to validate secs before casting: if secs < 0 then clamp to 0 (or return
an error if you prefer API change) and only then assign via uint32(secs) into
the pointer field so negative inputs cannot become ^uint32(0) or other
unintended values.

client/internal/peer/guard/guard.go (1)

57-62: ⚡ Quick win

SetOnNetworkChange after Start() is an undetected data race

The comment says "Set once before Start() is called; no lock needed." This is safe only if the goroutine spawning Start() happens strictly after SetOnNetworkChange returns, establishing a happens-before edge. However, nothing in the API enforces this — a caller that invokes SetOnNetworkChange after spawning go g.Start(...) introduces a data race on g.onNetworkChange with no diagnostic. Consider using sync/atomic to store the function pointer, or document this precondition with a //nolint:gocritic + go vet note so it surfaces in race-detector runs.

As a lightweight alternative, a sync/atomic.Pointer[func()] store/load pair makes the intent explicit and race-detector-clean with minimal overhead.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/guard/guard.go` around lines 57 - 62, SetOnNetworkChange
currently writes g.onNetworkChange without synchronization which can race with
concurrent Start() readers; change the Guard implementation to store the
callback via a sync/atomic pointer type (e.g. sync/atomic.Pointer[func()])
instead of the plain field and update all reads (e.g. in Start and any other
goroutines that invoke the callback) to load via atomic.Load/Pointer (or the
generic Atomic.Pointer API) before invoking; ensure SetOnNetworkChange uses
atomic.Store/Pointer to set the callback and nil-checks use the atomic load so
access is race-detector-clean and retains the same semantics.

client/internal/peer/ice_backoff.go (2)

82-92: 💤 Low value

Optional: clear suspended flag on lazy expiry to keep state self-consistent.

IsSuspended() returns false once nextRetry is in the past, but it leaves s.suspended = true. Any future caller that inspects the raw flag (or that adds non-time-based logic) will see a stale "suspended" state. The condition is duplicated in Snapshot() (line 131) and the two helpers can drift. Consider clearing the flag once it has lapsed — pure read API can become a write (still safe under s.mu), or simply expose only the time-based predicate.

 func (s *iceBackoffState) IsSuspended() bool {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 	if !s.suspended {
 		return false
 	}
-	if time.Now().After(s.nextRetry) {
-		return false
-	}
-	return true
+	if time.Now().After(s.nextRetry) {
+		s.suspended = false
+		return false
+	}
+	return true
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/ice_backoff.go` around lines 82 - 92, IsSuspended()
currently returns false when nextRetry has passed but leaves s.suspended true,
causing inconsistent state with Snapshot(); update IsSuspended() (in
iceBackoffState) to, under s.mu lock, check if s.suspended &&
time.Now().After(s.nextRetry) and if so clear s.suspended (and optionally reset
nextRetry) before returning false so the in-memory flags remain self-consistent;
ensure you modify only inside the locked section to avoid races and keep
Snapshot() behavior aligned.

---

161-169: 💤 Low value

Confirm: SetMaxBackoff resets the in-flight exponential schedule.

The doc comment states the failure counter is preserved, which it is, but buildBackoff calls bo.Reset() so the schedule restarts at InitialInterval. A peer that has climbed to e.g. ~8 m suspends will, after a server-pushed SetMaxBackoff to a different cap, drop back to ~1 m on the next failure. This is most likely intentional (new policy from server ⇒ fresh schedule), but the comment "preserves the failure counter" can be misread as "preserves the schedule"; consider clarifying.

-// SetMaxBackoff updates the cap. Called from ConnMgr.UpdatedRemotePeerConfig
-// when the server pushes a new value. Rebuilds the internal backoff with
-// the new schedule but preserves the failure counter.
+// SetMaxBackoff updates the cap. Called from ConnMgr.UpdatedRemotePeerConfig
+// when the server pushes a new value. Rebuilds the internal exponential
+// schedule from scratch (so the next markFailure starts at InitialInterval)
+// but preserves the cumulative failure counter for status reporting.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/ice_backoff.go` around lines 161 - 169, Update the
comment for SetMaxBackoff on iceBackoffState to clarify that although the
failure counter is preserved, the exponential backoff schedule is reset because
SetMaxBackoff calls buildBackoff(d) which invokes bo.Reset(); explicitly state
that the schedule (current interval) restarts at InitialInterval while the
failure count remains unchanged so callers won't be misled into thinking the
in-flight schedule is preserved.

client/internal/peer/conn.go (2)

1030-1056: 💤 Low value

Minor: redundant IsSuspended() check.

AttachICE checks iceBackoff.IsSuspended() (line 1034) and then calls attachICEListenerLocked, which performs the same check (line 1070). The first check returns early without an error; the second returns false (no-op). The behaviour is identical, so the first block is just an extra log path that fires before the listener attach. If the dual logging is intentional (different message), keep it; otherwise drop the outer check and rely on attachICEListenerLocked returning false to skip the SendOffer.

 func (conn *Conn) AttachICE() error {
 	conn.mu.Lock()
 	defer conn.mu.Unlock()
 
-	if conn.iceBackoff != nil && conn.iceBackoff.IsSuspended() {
-		snap := conn.iceBackoff.Snapshot()
-		conn.Log.Debugf("ICE backoff active (failure #%d, retry at %s), staying on relay",
-			snap.Failures,
-			snap.NextRetry.Format("15:04:05"))
-		return nil
-	}
 	if conn.handshaker == nil {
 		return fmt.Errorf("AttachICE: handshaker not initialized (Open not called)")
 	}
 	if conn.workerICE == nil {
 		return fmt.Errorf("AttachICE: workerICE is nil (relay-forced mode)")
 	}
 
 	if !conn.attachICEListenerLocked() {
 		return nil
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/conn.go` around lines 1030 - 1056, Remove the redundant
iceBackoff.IsSuspended() check and its logging from Conn.AttachICE and rely on
attachICEListenerLocked to handle suspended backoff (so AttachICE no longer
early-returns for suspension); specifically, delete the block that checks
conn.iceBackoff != nil && conn.iceBackoff.IsSuspended() and its
conn.Log.Debugf/return, leaving the rest of AttachICE (including the handshaker
nil checks and the call to attachICEListenerLocked and SendOffer) intact so
behavior is unchanged except for avoiding duplicate suspension logging.

---

19-20: 💤 Low value

Import group ordering.

shared/connectionmode is wedged between client/internal/metrics and client/internal/peer/conntype, breaking the client/... group's alphabetical ordering. goimports would put shared/... after the client/... group. Minor, but easy to fix.

 	"github.com/netbirdio/netbird/client/internal/metrics"
-	"github.com/netbirdio/netbird/shared/connectionmode"
 	"github.com/netbirdio/netbird/client/internal/peer/conntype"
 	"github.com/netbirdio/netbird/client/internal/peer/dispatcher"
 	"github.com/netbirdio/netbird/client/internal/peer/guard"
 	icemaker "github.com/netbirdio/netbird/client/internal/peer/ice"
 	"github.com/netbirdio/netbird/client/internal/peer/id"
 	"github.com/netbirdio/netbird/client/internal/peer/worker"
 	"github.com/netbirdio/netbird/client/internal/portforward"
 	"github.com/netbirdio/netbird/client/internal/stdnet"
 	"github.com/netbirdio/netbird/route"
 	relayClient "github.com/netbirdio/netbird/shared/relay/client"
+	"github.com/netbirdio/netbird/shared/connectionmode"

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/conn.go` around lines 19 - 20, The import block in
client/internal/peer/conn.go has shared/connectionmode placed between
client/internal packages, breaking the client/... group's alphabetical grouping;
reorder the imports so all client/internal/* imports (e.g.,
client/internal/metrics and client/internal/peer/conntype) are contiguous and
alphabetically ordered, and move
github.com/netbirdio/netbird/shared/connectionmode after the client/... group
(matching goimports grouping).

client/internal/peer/env_test.go (1)

33-34: 💤 Low value

Use constants instead of hardcoded environment variable names.

Lines 31–32 already reference EnvKeyNBConnectionMode and EnvKeyNBForceRelay. Lines 33–34 hardcode "NB_ENABLE_EXPERIMENTAL_LAZY_CONN" and "NB_LAZY_CONN_INACTIVITY_THRESHOLD" as string literals.

The peer package defines these as private constants envEnableLazyConn and envInactivityThreshold (accessible within the same package), or import EnvEnableLazyConn and EnvInactivityThreshold from client/internal/lazyconn. Using constants ensures any rename is caught at compile time.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/env_test.go` around lines 33 - 34, Replace the hardcoded
environment variable names in the test t.Setenv calls with the package
constants: use envEnableLazyConn instead of "NB_ENABLE_EXPERIMENTAL_LAZY_CONN"
and envInactivityThreshold instead of "NB_LAZY_CONN_INACTIVITY_THRESHOLD" (these
constants are defined in the peer package); update the two t.Setenv calls in
env_test.go to reference those constants so renames are caught at compile time.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

client/internal/peer/guard/guard.go (1)

144-147: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Synchronous onNetworkChange() still blocks the guard select loop

The callback is still invoked synchronously inside the srReconnectedChan branch. Until it returns, ctx.Done(), relayedConnDisconnected, iCEConnDisconnected, and any further srReconnectedChan events are all blocked. This was flagged in the previous review and remains unresolved.

🛠 Proposed fix (async dispatch with single-flight coalescing)

+		// networkChangePending prevents redundant concurrent recreations.
+		var networkChangePending atomic.Bool

 		case <-srReconnectedChan:
 			g.log.Debugf("has network changes, reset reconnection ticker")
 			ticker.Stop()
 			ticker = g.newReconnectTicker(ctx)
 			tickerChannel = ticker.C
 			iceState.reset()
-			// Phase 3.5 (`#5989`): notify Conn to reset iceBackoff + recreate workerICE
-			if g.onNetworkChange != nil {
-				g.onNetworkChange()
-			}
+			// Phase 3.5 (`#5989`): notify Conn to reset iceBackoff + recreate workerICE.
+			// Dispatched asynchronously so the guard loop stays responsive to ctx.Done().
+			if g.onNetworkChange != nil && !networkChangePending.Swap(true) {
+				go func() {
+					defer networkChangePending.Store(false)
+					g.onNetworkChange()
+				}()
+			}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/guard/guard.go` around lines 144 - 147, The
srReconnectedChan branch currently calls g.onNetworkChange() synchronously
inside the guard select loop, which blocks other cases; change it to dispatch
the work asynchronously: when handling srReconnectedChan, if g.onNetworkChange
!= nil spawn a goroutine to invoke it instead of calling it inline and use a
single-flight/coalescing mechanism (e.g., a singleflight.Group or an in-struct
boolean guarded by a mutex/atomic) so concurrent srReconnectedChan events only
trigger one inflight onNetworkChange invocation; ensure the async worker
performs the existing reset iceBackoff and recreate workerICE logic and that you
respect ctx cancellation to avoid leaking goroutines.

client/internal/peer/worker_ice.go (1)

218-230: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Synchronize lastKnownState across the callback and network-change paths.

IsConnected() at Lines 227-230 now drives the skip-close branch, but onConnectionStateChange() still writes lastKnownState without the same synchronization at Lines 550 and 562. That leaves a data race on the exact close-or-skip decision this PR added.

#!/bin/bash
rg -n -C2 '\blastKnownState\b' client/internal/peer/worker_ice.go

Also applies to: 545-563

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/worker_ice.go` around lines 218 - 230, IsConnected reads
lastKnownState under w.muxAgent while onConnectionStateChange writes
lastKnownState without locking, creating a race; fix by protecting all accesses
to lastKnownState with the same mutex (w.muxAgent) — update
onConnectionStateChange (the callback that sets lastKnownState at/around where
it currently assigns at lines ~545-563) to acquire w.muxAgent.Lock() and defer
w.muxAgent.Unlock() around writes (and any reads), ensuring symmetry with
IsConnected and preventing the close-or-skip race.

client/internal/conn_mgr.go (2)

282-320: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Apply timeout pushes even when the managed mode itself doesn’t change.

Lines 282-320 only restart the manager on modeChanged, but initLazyManager() consumes relayTimeoutSecs and p2pTimeoutSecs once at Lines 557-562. A push from, for example, p2p-dynamic(60/30) to p2p-dynamic(10/5) updates the fields but leaves the live manager running with the old thresholds until a full restart.

Also applies to: 553-562

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/conn_mgr.go` around lines 282 - 320, The runtime ignores
timeout-only pushes if the mode doesn’t change; detect when
relayTimeoutSecs/p2pTimeoutSecs/p2pRetryMaxSecs change while a manager is active
and apply them by restarting or updating the manager: in conn_mgr.go where
modeChanged/wasManaged/isManaged are computed (and where lazyConnMgr is
checked), add a branch that if isManaged && e.lazyConnMgr != nil and any of
relayTimeoutSecs/p2pTimeoutSecs/p2pRetryMaxSecs differ from the manager’s
current thresholds, call e.closeManager(ctx) then e.initLazyManager(ctx),
e.startModeSideEffects(), and e.addPeersToLazyConnManager() (or alternatively
propagate the new values into the running manager); reference modeUsesLazyMgr,
lazyConnMgr, relayTimeoutSecs, p2pTimeoutSecs, p2pRetryMaxSecs,
propagateP2pRetryMaxToConns, initLazyManager, closeManager,
startModeSideEffects, and addPeersToLazyConnManager when making the change.

---

181-188: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t leave e.mode in lazy/dynamic when Rosenpass prevents the manager from starting.

Lines 185-187 and 313-315 return early, but e.mode is still kept/set to p2p-lazy or p2p-dynamic. New peers then inherit that mode, AddPeerConn() falls back to conn.Open() at Lines 391-395 because no manager exists, and ModeP2PDynamic still defers ICE listener registration in client/internal/peer/conn.go at Lines 239-242. Since ActivatePeer() also returns early with no manager at Lines 449-451, those peers can stay relay-only indefinitely.

Also applies to: 312-320

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/conn_mgr.go` around lines 181 - 188, When you bail out from
starting the lazy/dynamic manager (both the modeUsesLazyMgr() check and the
e.rosenpassEnabled check), clear/unset the lazy P2P mode on e.mode so it no
longer advertises "p2p-lazy" or "p2p-dynamic" to new peers; specifically, before
each early return set e.mode to a non-lazy value (i.e. remove the lazy/dynamic
flag or assign the normal/direct P2P mode) so callers like AddPeerConn() and
ActivatePeer() won’t treat peers as manager-backed (avoiding the conn.Open()
fallback and deferred ICE registration in ModeP2PDynamic).

client/internal/peer/conn.go (1)

202-212: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep the explicit “disable backoff” state round-trippable.

propagateP2pRetryMaxToConns() converts the wire sentinel to time.Duration(0), but Line 1159 stores that back as plain 0. After a later reopen, Lines 204-207 reinterpret 0 as “use default”, so a user-explicit disable silently turns back into the 15-minute cap.

Also applies to: 1156-1162

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/conn.go` around lines 202 - 212, The explicit "disable
backoff" sentinel is being lost because 0 is being reused for "use default";
introduce and use a distinct sentinel (e.g. const P2PRetryMaxDisabledSeconds =
-1) and update both propagateP2pRetryMaxToConns() (the code around 1156-1162)
and this initialization block in conn.go to interpret -1 as "disabled" (set
backoffCap to 0 or disable the iceBackoff entirely) while treating 0 as "use
built-in default" (map to DefaultP2PRetryMax); also ensure
newIceBackoff()/iceBackoff.SetMaxBackoff accept/handle the disabled sentinel so
the explicit disable round-trips unchanged.

client/ui/client_ui.go (1)

656-666: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Do not silently coerce invalid timeout input to 0.

Invalid/overflow timeout text currently collapses to 0, which can unintentionally clear overrides when saving settings. Parse should return an error and buildSetConfigRequest should surface it.

Proposed fix

-func parseUint32Field(text string) uint32 {
+func parseUint32Field(text string) (uint32, error) {
 	t := strings.TrimSpace(text)
 	if t == "" {
-		return 0
+		return 0, nil
 	}
 	v, err := strconv.ParseUint(t, 10, 32)
 	if err != nil {
-		return 0
+		return 0, fmt.Errorf("must be a whole number between 0 and %d", uint64(^uint32(0)))
 	}
-	return uint32(v)
+	return uint32(v), nil
 }
@@
-	relaySecs := parseUint32Field(s.iRelayTimeout.Text)
-	p2pSecs := parseUint32Field(s.iP2pTimeout.Text)
-	retrySecs := parseUint32Field(s.iP2pRetryMax.Text)
+	relaySecs, err := parseUint32Field(s.iRelayTimeout.Text)
+	if err != nil {
+		return nil, fmt.Errorf("invalid relay timeout: %w", err)
+	}
+	p2pSecs, err := parseUint32Field(s.iP2pTimeout.Text)
+	if err != nil {
+		return nil, fmt.Errorf("invalid P2P timeout: %w", err)
+	}
+	retrySecs, err := parseUint32Field(s.iP2pRetryMax.Text)
+	if err != nil {
+		return nil, fmt.Errorf("invalid P2P retry-max: %w", err)
+	}
 	req.RelayTimeoutSeconds = &relaySecs
 	req.P2PTimeoutSeconds = &p2pSecs
 	req.P2PRetryMaxSeconds = &retrySecs

Also applies to: 741-750

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/ui/client_ui.go` around lines 656 - 666, The parseUint32Field function
currently swallows parse/overflow errors and returns 0 which can accidentally
clear saved timeouts; change parseUint32Field to return (uint32, error) instead
of silently coercing — validate strings.TrimSpace(text) then call
strconv.ParseUint(..., 32) and return an explicit error on failure (including
overflow), and update all callers (notably buildSetConfigRequest) to propagate
or surface that error to the caller (e.g., return/build an error response
instead of using 0); apply the same change to the other similar parser at the
741-750 block so all timeout parsing surfaces errors instead of returning 0.

client/android/preferences.go (1)

349-352: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard negative timeout inputs before casting to uint32.

Negative values currently wrap into very large uint32 values and can silently corrupt timeout overrides. Treat <= 0 as “clear override” (0) before casting.

Proposed fix

 func (p *Preferences) SetRelayTimeoutSeconds(secs int64) {
+	if secs <= 0 {
+		v := uint32(0)
+		p.configInput.RelayTimeoutSeconds = &v
+		return
+	}
 	v := uint32(secs)
 	p.configInput.RelayTimeoutSeconds = &v
 }
@@
 func (p *Preferences) SetP2pTimeoutSeconds(secs int64) {
+	if secs <= 0 {
+		v := uint32(0)
+		p.configInput.P2pTimeoutSeconds = &v
+		return
+	}
 	v := uint32(secs)
 	p.configInput.P2pTimeoutSeconds = &v
 }
@@
 func (p *Preferences) SetP2pRetryMaxSeconds(secs int64) {
+	if secs <= 0 {
+		v := uint32(0)
+		p.configInput.P2pRetryMaxSeconds = &v
+		return
+	}
 	v := uint32(secs)
 	p.configInput.P2pRetryMaxSeconds = &v
 }

Also applies to: 370-373, 390-393

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/android/preferences.go` around lines 349 - 352, Guard against negative
timeout inputs in SetRelayTimeoutSeconds: treat any secs <= 0 as a clear
override by using 0 instead of allowing a negative int64 to wrap when cast to
uint32. Update SetRelayTimeoutSeconds to check secs <= 0 and set the config
pointer to a uint32(0) in that case; otherwise cast secs to uint32 and assign as
before. Apply the same pattern to the other timeout setter functions in this
file that perform the uint32 cast so they also handle non-positive inputs
safely.

client/internal/lazyconn/manager/manager.go (1)

68-68: ⚡ Quick win

Remove unused inactivityThreshold field from Manager struct.

The field is assigned in NewManager (line 93) but never read; all timeout logic flows through resolvedTimeouts(). It's dead code from the migration and can be safely removed.

🧹 Removal diff

 type Manager struct {
 	engineCtx           context.Context
 	peerStore           *peerstore.Store
-	inactivityThreshold time.Duration
 
 	managedPeers         map[string]*lazyconn.PeerConfig

 	m := &Manager{
 		engineCtx:            engineCtx,
 		peerStore:            peerStore,
-		inactivityThreshold:  inactivity.DefaultInactivityThreshold,
 		managedPeers:         make(map[string]*lazyconn.PeerConfig),

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/lazyconn/manager/manager.go` at line 68, Remove the dead
inactivityThreshold field from the Manager struct and eliminate its assignment
in NewManager: delete the inactivityThreshold declaration in the Manager type,
remove any parameter/argument or line in NewManager that sets
m.inactivityThreshold, and ensure no other code reads it (timeout logic should
remain using resolvedTimeouts()). Update related constructors/call sites to stop
passing that value and run tests to confirm nothing else references
inactivityThreshold.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

client/cmd/service_installer.go (1)

135-139: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail install/reconfigure when profile persistence fails

Line 137 and Line 263 currently downgrade persistence failure to a warning, which can complete the command with user-requested connection-mode/timeout settings silently not applied.

Suggested fix

-		if err := applyConnectionModeFlagsToProfile(cmd); err != nil {
-			cmd.PrintErrf("Warning: failed to persist connection-mode flags: %v\n", err)
-		}
+		if err := applyConnectionModeFlagsToProfile(cmd); err != nil {
+			return fmt.Errorf("persist connection-mode flags: %w", err)
+		}
...
-		if err := applyConnectionModeFlagsToProfile(cmd); err != nil {
-			cmd.PrintErrf("Warning: failed to persist connection-mode flags: %v\n", err)
-		}
+		if err := applyConnectionModeFlagsToProfile(cmd); err != nil {
+			return fmt.Errorf("persist connection-mode flags: %w", err)
+		}

Also applies to: 263-265

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/cmd/service_installer.go` around lines 135 - 139, The code currently
logs a warning when applyConnectionModeFlagsToProfile(cmd) fails (via
cmd.PrintErrf) which allows install/reconfigure to succeed without persisting
user-requested connection-mode/timeout settings; change this so the error is
returned (or causes the command to fail) instead of being downgraded to a
warning: locate the call to applyConnectionModeFlagsToProfile in
service_installer.go and replace the cmd.PrintErrf warning path with logic that
returns the error (or wraps it with context) from the enclosing function so the
installer/reconfigure aborts on persistence failure; apply the same change for
the other occurrence around lines 263-265 to ensure both install and reconfigure
fail on persistence errors.

client/internal/peer/worker_ice.go (1)

218-231: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Data race on lastKnownState is still present and now matters more.

This is the same concern raised earlier: IsConnected() reads w.lastKnownState under muxAgent, but the onConnectionStateChange callback writes it at lines 550 and 562 without holding the mutex. With the new Conn.onNetworkChange path using IsConnected() to decide whether to skip workerICE.Close(), a torn or stale read (or simply a race-detector failure in CI) can:

• skip Close on an agent that has just transitioned away from Connected — leaving a dead agent in place across a network change, or
• close an agent that is still in Connected — the exact case the new "skip when connected" path is meant to avoid.

Promote lastKnownState to atomic.Int32 (or guard the writes with muxAgent); the existing reads in IsConnected/state-change branches map cleanly to atomic load/store.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/worker_ice.go` around lines 218 - 231, The data race is
on WorkerICE.lastKnownState: IsConnected() reads it under muxAgent while the
onConnectionStateChange callback updates it without the mutex; change
lastKnownState to an atomic (e.g., atomic.Int32) and replace reads/writes in
IsConnected and both onConnectionStateChange sites (the assignments at the two
callback locations) with atomic load/store operations, or alternatively move
those writes to be under muxAgent.Lock()/Unlock()—ensure all accesses use the
same atomic or mutex scheme so IsConnected, onConnectionStateChange, and any
other reader/writer (WorkerICE, IsConnected, onConnectionStateChange) are
consistent.

client/internal/conn_mgr.go (2)

692-700: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

ServerPushedP2pRetryMaxSecs masking 0 with the daemon default still contradicts the "not yet received" semantics used in server.go.

This is the same concern as the earlier review: the comment in server.go GetConfig (lines 1518–1522) says all server-pushed values are zero/empty when no PeerConfig has been received, but this getter substitutes DefaultP2PRetryMax whenever the stored value is 0. As a result, spP2pRetMax is never 0 once the engine is running, so the UI has no reliable "server hasn't pushed a value yet" sentinel for this field — unlike ServerPushedP2pTimeoutSecs and ServerPushedRelayTimeoutSecs, which return raw zeros.

Either move the default-fallback into a separate helper (e.g., ServerPushedP2pRetryMaxSecsOrDefault()) consumed by the UI, or document very clearly that this getter intentionally returns the daemon default and update the server.go comment to match. As written today the two diverge.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/conn_mgr.go` around lines 692 - 700,
ServerPushedP2pRetryMaxSecs currently masks a stored zero with
peer.DefaultP2PRetryMax which breaks the "zero means not yet received"
convention used in server.go; change this by making ServerPushedP2pRetryMaxSecs
return the raw stored value (no default masking) and add a new helper named
ServerPushedP2pRetryMaxSecsOrDefault() that returns the stored value or
peer.DefaultP2PRetryMax when zero, then update callers (UI should use the
OrDefault helper) or alternatively update the server.go comment if you
intentionally want the getter to supply the daemon default; locate methods by
name ServerPushedP2pRetryMaxSecs and add the new helper with identical locking
(spMu) semantics.

---

277-323: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Same-mode timeout/retry-max changes still don't restart the lazy manager.

This is the same concern raised earlier in the review history: when only the timeouts (or p2pRetryMaxSecs) change inside the same managed mode (e.g., p2p-dynamic 60s → 10s, mode unchanged), the early-return at lines 282–284 is bypassed but then neither the modeChanged && wasManaged && isManaged branch (lines 304–310) nor the lazyConnMgr == nil branch (lines 312–321) fires. The new relayTimeoutSecs / p2pTimeoutSecs are stored on ConnMgr but never applied to the running manager.Manager, since initLazyManager is the only consumer of those fields.

Capture timeoutsChanged before overwriting the old fields and trigger a restart in the same-mode managed case as well:

♻️ Suggested fix

-	prev := e.mode
-	e.mode = newMode
-	e.relayTimeoutSecs = newRelay
-	e.p2pTimeoutSecs = newP2P
-	e.p2pRetryMaxSecs = newP2pRetry
-	e.propagateP2pRetryMaxToConns()
+	prev := e.mode
+	timeoutsChanged := newRelay != e.relayTimeoutSecs || newP2P != e.p2pTimeoutSecs
+	e.mode = newMode
+	e.relayTimeoutSecs = newRelay
+	e.p2pTimeoutSecs = newP2P
+	e.p2pRetryMaxSecs = newP2pRetry
+	e.propagateP2pRetryMaxToConns()

 	wasManaged := modeUsesLazyMgr(prev)
 	isManaged := modeUsesLazyMgr(newMode)
 	modeChanged := prev != newMode

 	if modeChanged && wasManaged && !isManaged {
 		log.Infof("lazy/dynamic connection manager disabled by management push (mode=%s)", newMode)
 		e.closeManager(ctx)
 		e.statusRecorder.UpdateLazyConnection(false)
 		return nil
 	}

-	if modeChanged && wasManaged && isManaged {
-		// Switching between lazy and dynamic at runtime: tear down the
-		// existing manager so initLazyManager picks up the new timeouts.
-		log.Infof("lazy/dynamic mode change %s -> %s, restarting manager", prev, newMode)
+	if wasManaged && isManaged && (modeChanged || timeoutsChanged) {
+		log.Infof("restarting lazy/dynamic manager to apply updated mode/timeouts (%s -> %s, timeouts-changed=%t)",
+			prev, newMode, timeoutsChanged)
 		e.closeManager(ctx)
 		e.statusRecorder.UpdateLazyConnection(false)
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/conn_mgr.go` around lines 277 - 323, The code updates
connection timeouts but doesn't restart the running lazy manager when only
timeouts/p2pRetryMaxSecs change while remaining in a managed mode; capture
whether timeouts changed before you overwrite fields (compare
newRelay/newP2P/newP2pRetry to
e.relayTimeoutSecs/e.p2pTimeoutSecs/e.p2pRetryMaxSecs into a timeoutsChanged
boolean) and, after computing wasManaged/isManaged, if wasManaged && isManaged
&& timeoutsChanged then log a restart, call e.closeManager(ctx),
e.statusRecorder.UpdateLazyConnection(false), and then re-init the manager via
e.initLazyManager(ctx) followed by e.startModeSideEffects() and return
e.addPeersToLazyConnManager() so the new timeouts are applied to the running
manager (use existing functions resolveConnectionMode, modeUsesLazyMgr,
initLazyManager, closeManager, propagateP2pRetryMaxToConns,
startModeSideEffects, addPeersToLazyConnManager to locate code).

client/internal/conn_mgr_test.go (1)

206-221: 💤 Low value

Race-safety test only catches the race when run under -race.

The test asserts no crash but no values. Without go test -race, a missing lock would not produce a failure here. Consider adding a comment so future readers know this guard relies on the race detector, and optionally have at least one assertion (e.g., final value is one of the writes) so the test is not a no-op without -race.

♻️ Suggested clarification

+// Run with `-race` to validate concurrent reads/writes on serverPushed* fields.
 func TestConnMgr_ServerPushedFieldsAreRaceSafe(t *testing.T) {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/conn_mgr_test.go` around lines 206 - 221, Update
TestConnMgr_ServerPushedFieldsAreRaceSafe to document that it relies on the Go
race detector and to make it meaningful without -race: add a short comment above
the test mentioning that the test is intended to catch races only when run with
`-race` and references the guarded fields (spMu, serverPushedRelayTimeoutSecs)
and accessor ServerPushedRelayTimeoutSecs(); then add a lightweight assertion
after the goroutine completes that the final value returned by
cm.ServerPushedRelayTimeoutSecs() is within the range of expected writes (e.g.,
0..999) or equals the last written value to ensure the test is not a no-op when
run without the race detector.

client/internal/conn_mgr.go (1)

196-214: 💤 Low value

startModeSideEffects can spawn the dynamic-inactivity loop with a nil lazyCtx.

startModeSideEffects is called both from Start (after initLazyManager, which sets e.lazyCtx) and from UpdatedRemotePeerConfig line 319 (also after initLazyManager). So today the contract holds. But the helper itself does not guard against being invoked outside that ordering, and a future caller could easily miss it. Since runDynamicInactivityLoop already null-checks lazyConnMgr and InactivityManager(), consider adding a parallel guard for e.lazyCtx == nil (or, simpler, branching on e.isStartedWithLazyMgr() before spawning) so the goroutine cannot be launched in an undefined state.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/conn_mgr.go` around lines 196 - 214, startModeSideEffects can
start the dynamic-inactivity goroutine when e.lazyCtx is nil; fix it by guarding
the dynamic branch so the goroutine is only spawned when the lazy manager is
initialized (e.g. check e.isStartedWithLazyMgr() or e.lazyCtx != nil) before
calling e.wg.Add(1) and launching the goroutine that calls
runDynamicInactivityLoop(e.lazyCtx); keep the existing UpdateLazyConnection call
for ModeP2PLazy but ensure ModeP2PDynamic only proceeds when initLazyManager has
been run (refer to startModeSideEffects, runDynamicInactivityLoop,
initLazyManager, UpdatedRemotePeerConfig, and e.lazyCtx).

client/internal/peer/ice_backoff.go (1)

161-169: 💤 Low value

SetMaxBackoff discards the in-flight backoff progress on every change.

buildBackoff does an unconditional bo.Reset(), so any active doubling progress is lost when the management server pushes a new cap. In the common path (cap goes up or down at peer steady state) this is fine, but if a peer is mid-failure-storm and the cap is changed to a smaller-but-nonzero value, the next markFailure restarts at InitialInterval (~1m) instead of clamping the in-flight delay to the new cap. If that surprises operators, document it — or, better, only rebuild when the new cap is smaller than the current nextRetry delta.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/ice_backoff.go` around lines 161 - 169, SetMaxBackoff
currently throws away in-flight backoff progress by unconditionally calling
buildBackoff(d); change it to only rebuild the backoff when the new cap is
smaller than the current pending retry interval so in-flight doubling is
preserved otherwise. Concretely, inside iceBackoffState.SetMaxBackoff check the
current next retry delta from the existing s.bo (e.g. via the backoff
implementation's NextBackOff/Peek or a saved next value used by markFailure); if
d >= currentNext then just update s.maxBackoff = d and return, otherwise rebuild
s.bo = buildBackoff(d) and set s.maxBackoff = d so an ongoing storm will be
clamped down only when the new cap is smaller than the in-flight delay. Ensure
you use the same symbols: SetMaxBackoff, s.maxBackoff, s.bo, buildBackoff and
keep behavior for equal durations as before.

client/internal/peer/handshaker.go (1)

148-153: 💤 Low value

Minor: RemoteICESupported() is evaluated twice per dispatch; the second call could reorder around readICEListener().

RemoteICESupported() is an atomic load and cheap, but re-evaluating it after readICEListener() opens a tiny window where the remote-ICE state flipped between the two calls (e.g., remote stops supporting ICE just before the debug log is decided). It would not change correctness, but caching the value once per branch makes the intent clearer and the log message more accurate:

♻️ Suggested cleanup

-			if iceListener := h.readICEListener(); iceListener != nil && h.RemoteICESupported() {
-				iceListener(&remoteOfferAnswer)
-			} else if h.RemoteICESupported() {
-				h.log.Debugf("remote OFFER (session %s) without local ICE listener (relay-forced mode or peer in ICE backoff)",
-					remoteOfferAnswer.SessionIDString())
-			}
+			if remoteICE := h.RemoteICESupported(); remoteICE {
+				if iceListener := h.readICEListener(); iceListener != nil {
+					iceListener(&remoteOfferAnswer)
+				} else {
+					h.log.Debugf("remote OFFER (session %s) without local ICE listener (relay-forced mode or peer in ICE backoff)",
+						remoteOfferAnswer.SessionIDString())
+				}
+			}

Also applies to: 173-178

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/internal/peer/handshaker.go` around lines 148 - 153, Cache the result
of h.RemoteICESupported() in a local variable and reuse it instead of calling
RemoteICESupported() twice around readICEListener(); e.g., evaluate
remoteICESupported := h.RemoteICESupported() before calling h.readICEListener()
and use remoteICESupported in the if/else condition and in the h.log.Debugf
branch to avoid a race/reordering window; apply the same change to the similar
block around lines referencing readICEListener() and RemoteICESupported() (the
second occurrence mentioned).